Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(validation): validation fails if component has source_config #337

Conversation

markkovari
Copy link
Contributor

Feature or Problem

Related Issues

#288

Release Information

Consumer Impact

Testing

Unit Test(s)

unit tests should cover the negative case, other validation calls don't have this error

Acceptance or Integration

Manual Verification

@markkovari
Copy link
Contributor Author

markkovari commented Jul 17, 2024

I do have 2 questions tho:

  • is it even a valid configuration, if not feel free to point out edge cases to add
  • the validation looks similar to a callback hell, hopefully you have wide enough screens, please call me out if it needs refactor (update: definitely needs to be refactored, I forgot how to rust 😿 )

Appreciate

Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @markkovari ! I think the structure of this PR looks great, you may just need to consider a change or two after #307. If we want to try and move this forward now, we should at least just change the parsing of the source_config key to be in a link trait

crates/wadm-types/src/validation.rs Outdated Show resolved Hide resolved
@brooksmtownsend
Copy link
Member

Alright @markkovari you should be good to proceed with this one 😄 Just take note of the newer structure where the source block is the recommended route, and source_config is a backwards compatible field. I would recommend just validating that the source: config: block isn't present, since we want to deprecate source_config out in a future release anyways

Copy link

stale bot commented Sep 22, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this has been closed too eagerly, please feel free to tag a maintainer so we can keep working on the issue. Thank you for contributing to wasmCloud!

@stale stale bot added the stale label Sep 22, 2024
@joonas joonas removed the stale label Sep 24, 2024
@markkovari
Copy link
Contributor Author

Just a question here: since v0.13.0 the fields are deprecated (source_config) and (target_config), this is still relevant right?
Currently the yaml parsing into the structure is always None so it is doable by the casual yaml traversing/parsing method.
And this should be even related to this too:
wasmCloud/wasmCloud#2862

@brooksmtownsend
Copy link
Member

Hey @markkovari , yep I think it's still relevant here just failing when source.config is present instead of source_config if that makes sense 😄

It would be best to do the validation in a place where we can just use the same logic for both

@markkovari markkovari force-pushed the feat/validate-fails-with-source_config-on-components branch from f8a269d to 5089132 Compare December 10, 2024 21:45
@markkovari markkovari requested a review from a team as a code owner December 10, 2024 21:45
Copy link
Contributor

@thomastaylor312 thomastaylor312 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there @markkovari! I think there is a line in the test left over and the failure should be a warning as it is only a deprecated field

crates/wadm-types/src/validation.rs Outdated Show resolved Hide resolved
tests/validation.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@thomastaylor312 thomastaylor312 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops hit wrong button

@markkovari
Copy link
Contributor Author

Thanks will add this changes tomorrow early. But still not sure if this is the expected behavior or not. WADM is so much more magical than wasmCloud 😄

@thomastaylor312
Copy link
Contributor

This is a great start @markkovari. There is definitely more we could add to this part of validation, but even having something that warns about deprecation is great

@markkovari
Copy link
Contributor Author

markkovari commented Dec 17, 2024

@thomastaylor312 @brooksmtownsend sorry for the delay
unfortunately the solution is a bit hacky 😅 , but the serialization is always making the yaml parsed objects link traits' sounce_config and target_config None, not sure if it is possible to do it the "nice way" just like the other steps

Thanks

Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, any thoughts here @thomastaylor312 ? Looks like we can remove the validate_raw_yaml bit once we finally remove the source_config/target_config fields

@markkovari markkovari force-pushed the feat/validate-fails-with-source_config-on-components branch 2 times, most recently from b569776 to 3cd97cc Compare December 19, 2024 19:11
@markkovari markkovari force-pushed the feat/validate-fails-with-source_config-on-components branch from 3cd97cc to aed1811 Compare December 19, 2024 20:10
@thomastaylor312 thomastaylor312 enabled auto-merge (rebase) December 20, 2024 16:27
@thomastaylor312 thomastaylor312 merged commit 265f732 into wasmCloud:main Dec 20, 2024
5 checks passed
@markkovari markkovari deleted the feat/validate-fails-with-source_config-on-components branch January 6, 2025 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants